Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Soundcloud #186

Closed

Conversation

monicacheung
Copy link
Member

@monicacheung monicacheung commented Apr 14, 2016

Closes #2

You can enter a Soundcloud URL/trackID in the search bar or as a query string (?v=) in the URL and ZAP will play it.

Try these out in the ZAP search bar or ?v= in the URL:

There are changes to plyr.js in the directory bower_components/plyr to add support for Soundcloud to plyr. These are for test/debug until we send a PR to plyr.

Todo/Things that broke:

  • Fix the tweet message.
  • Fix storing current time of player and resume on returning to page.
  • Update the UI a bit to be indicative of soundcloud support.
  • More TODO in the code.

@@ -478,7 +481,7 @@
// Removed call to arguments.callee (used explicit function name instead)
function _extend() {
// Get arguments
var objects = arguments;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like tab indents either, but you need to revert these

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shakeelmohamed
Copy link
Member

shakeelmohamed commented Apr 14, 2016

Thanks @monicacheung, this is going to be a huge feature! I immediately noticed the demo button isn't hidden when a soundcloud track is playing, so we should fix that.

I've mainly left comments on the plyr changes, I need to go through your changes in everything.js again. We'll also need to update the UI a bit to be indicative of soundcloud support once we cleanup the code

/*
* Handle form submission.
*/
// TODO: Need to test this to make sure that nothing broke.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it may be time to add some proper tests... #6. In the absence of that, we should create a QA checklist for manual tests until then - it can live on the wiki.

that.setupPlyrToggle();
}
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to show an error for the else case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shakeelmohamed
Copy link
Member

Ping me when you're ready to finish this up, here are the high level steps:

  • Revert any changes to bower_components/plyr
  • Rebase your changes with master
  • Resolve merge conflicts (after the rebase this shouldn't be too bad)

Bonus: let's try to retain every commit 💯

@shakeelmohamed
Copy link
Member

I'll clean up the branch since @monicacheung isn't able to work on this anymore. Then anyone can finish up what's left

@shakeelmohamed
Copy link
Member

Closed in favor of #216, fixed many of the merge conflicts and reverted the plyr changes since ply has soundcloud support now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants